Skip to content

Rely on noMoreOperators for dynamic filters collection#12594

Merged
losipiuk merged 1 commit intotrinodb:masterfrom
raunaqmorarka:df-partitions
May 31, 2022
Merged

Rely on noMoreOperators for dynamic filters collection#12594
losipiuk merged 1 commit intotrinodb:masterfrom
raunaqmorarka:df-partitions

Conversation

@raunaqmorarka
Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka commented May 30, 2022

Relying on noMoreOperators from DynamicFilterSourceOperatorFactory
for detecting completion of dynamic filter collection will simplify
the implementation for fault tolerant execution where the collection
may take place in a source stage.

@cla-bot cla-bot Bot added the cla-signed label May 30, 2022
@raunaqmorarka raunaqmorarka mentioned this pull request May 30, 2022
Merged
losipiuk
losipiuk approved these changes May 30, 2022
View reviewed changes
Comment thread core/trino-main/src/main/java/io/trino/sql/planner/LocalDynamicFilterConsumer.java Outdated
Comment thread core/trino-main/src/main/java/io/trino/sql/planner/LocalDynamicFilterConsumer.java Outdated
Comment thread core/trino-main/src/main/java/io/trino/operator/DynamicFilterSourceOperator.java Outdated
Comment thread core/trino-main/src/main/java/io/trino/sql/planner/LocalDynamicFilterConsumer.java Outdated
@raunaqmorarka
Rely on noMoreOperators for dynamic filters collection
36b8c04 
Relying on noMoreOperators from DynamicFilterSourceOperatorFactory
for detecting completion of dynamic filter collection will simplify
the implementation for fault tolerant execution where the collection
may take place in a source stage.
@raunaqmorarka raunaqmorarka requested a review from losipiuk May 30, 2022 09:53
@sopel39
Copy link
Copy Markdown
Member

sopel39 commented May 30, 2022

Can it increase DF collection latency?

@raunaqmorarka
Copy link
Copy Markdown
Member Author

raunaqmorarka commented May 30, 2022

Can it increase DF collection latency?

Normally OperatorFactory#noMoreOperators should fire before or around when the last operator finishes. Do you see a reason to suspect impact to latency ? I can run the usual benchmarks to confirm if needed.

@sopel39
Copy link
Copy Markdown
Member

sopel39 commented May 30, 2022 •
edited
Loading

Normally OperatorFactory#noMoreOperators should fire before or around when the last operator finishes

I think we can bail-out faster before noMoreOperators is called. Idk, maybe it's not an issue. Haven't looked at the code for a while

@raunaqmorarka
Copy link
Copy Markdown
Member Author

raunaqmorarka commented May 31, 2022

Normally OperatorFactory#noMoreOperators should fire before or around when the last operator finishes

I think we can bail-out faster before noMoreOperators is called. Idk, maybe it's not an issue. Haven't looked at the code for a while

I've run TPC benchmarks and confirmed no regressions. Let me know if you have any input about the code or any problematic scenarios in mind.

@sopel39
Copy link
Copy Markdown
Member

sopel39 commented May 31, 2022

Let me know if you have any input about the code or any problematic scenarios in mind.

Nope. Just wanted to confirm latency is not harmed

@losipiuk losipiuk merged commit c49be84 into trinodb:master May 31, 2022
@github-actions github-actions Bot added this to the 383 milestone May 31, 2022
arhimondr
arhimondr reviewed May 31, 2022
View reviewed changes
Comment thread core/trino-main/src/main/java/io/trino/sql/planner/DynamicFilterSourceConsumer.java
import io.trino.spi.predicate.TupleDomain;
import io.trino.sql.planner.plan.DynamicFilterId;

public interface DynamicFilterSourceConsumer
Copy link
Copy Markdown
Contributor

@arhimondr arhimondr May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: DynamicFilterConsumer, or maybe simply drop this interface and override the method you need for testing?

@colebow colebow mentioned this pull request May 31, 2022
Merged
@colebow
Copy link
Copy Markdown
Member

colebow commented May 31, 2022

Do we want a release notes entry for this?

@raunaqmorarka raunaqmorarka deleted the df-partitions branch May 31, 2022 16:10
@raunaqmorarka
Copy link
Copy Markdown
Member Author

raunaqmorarka commented May 31, 2022

Do we want a release notes entry for this?

This is just an internal refactoring, no user facing change

@raunaqmorarka raunaqmorarka mentioned this pull request Aug 17, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sopel39 sopel39 Awaiting requested review from sopel39

@losipiuk losipiuk Awaiting requested review from losipiuk

1 more reviewer

@arhimondr arhimondr arhimondr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Milestone

383

Development

Successfully merging this pull request may close these issues.

5 participants

@raunaqmorarka @sopel39 @colebow @losipiuk @arhimondr